Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove persistence of PeerStore to DB #8617

Merged
merged 12 commits into from
Feb 23, 2023

Conversation

saketh-are
Copy link
Collaborator

@saketh-are saketh-are commented Feb 22, 2023

After the changes in #8579:

  • Connected nodes are persisted to DB via the ConnectionStore
  • Connections to those nodes are re-established upon restarting the node

Previously, we persisted the PeerStore to disk to support faster network bootstrapping after restarting a node. Now that the ConnectionStore serves that purpose, it is no longer necessary to persist the PeerStore. Unlike ConnectionStore, PeerStore contains unverified information and may contain spam shared by malicious nodes. This PR both removes unnecessary load on the DB and limits persistence of malicious data.

This PR

  • Removes all interaction between the PeerStore and the DB
  • Updates APIs of the PeerStore accordingly as it no longer produces DB read/write errors
  • Updates PeerStore tests accordingly

@saketh-are saketh-are marked this pull request as ready for review February 22, 2023 00:17
@saketh-are saketh-are requested a review from a team as a code owner February 22, 2023 00:17
@saketh-are saketh-are requested review from akhi3030 and pompon0 and removed request for akhi3030 February 22, 2023 00:17
Copy link
Contributor

@pompon0 pompon0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@pompon0 pompon0 requested a review from mm-near February 23, 2023 11:08
Copy link
Contributor

@mm-near mm-near left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - please also update the CHANGELOG.md

chain/network/src/store/schema/mod.rs Show resolved Hide resolved
@saketh-are saketh-are merged commit 58441c0 into near:master Feb 23, 2023
andrei-near pushed a commit that referenced this pull request Feb 28, 2023
* remove persistence of PeerStore to DB

* restore ban_store test

* remove dead variable touch_other

* delete_peers doesn't error

* restore check_ignore_blacklisted_peers test

* add high-level comment on PeerStore

* deprecate Peers column

* finish deprecating Peers column and add DB migration

* fix test referencing DBCol::Peers

* update CHANGELOG.md
near-bulldozer bot pushed a commit that referenced this pull request Mar 8, 2023
In #8617 we introduced new DB migration (deprecating Peers column). But before it we had another migration indicating creation of flat storage columns, currently guarded by compilation flag.

Actually, we should put "peers" migration **before** "flat state" migration, because for nodes which don't have flat state enabled, intermediate version in `match` will be missing. We didn't see it because we also didn't update `DB_VERSION`, which essentially means that nodes didn't trigger any migrations yet.

Here I also bump `DB_VERSION`, so nodes will run "peers" migration and then "flat state" migration if compilation flag is enabled. The only outcome is that our custom nodes which have flat state **will not** run "peers" migration, but this is fine.

cc @saketh-are
marcelo-gonzalez added a commit to marcelo-gonzalez/nearcore that referenced this pull request Mar 11, 2023
this is a fix similar to
near#8711. After
near#8617, we need to correctly set
the boot nodes in this test, otherwise we get a timeout error in
test_tx_status() after the nodes are restarted
near-bulldozer bot pushed a commit that referenced this pull request Mar 14, 2023
this is a fix similar to
#8711. After
#8617, we need to correctly set the boot nodes in this test, otherwise we get a timeout error in test_tx_status() after the nodes are restarted
nikurt pushed a commit to nikurt/nearcore that referenced this pull request Mar 15, 2023
In near#8617 we introduced new DB migration (deprecating Peers column). But before it we had another migration indicating creation of flat storage columns, currently guarded by compilation flag.

Actually, we should put "peers" migration **before** "flat state" migration, because for nodes which don't have flat state enabled, intermediate version in `match` will be missing. We didn't see it because we also didn't update `DB_VERSION`, which essentially means that nodes didn't trigger any migrations yet.

Here I also bump `DB_VERSION`, so nodes will run "peers" migration and then "flat state" migration if compilation flag is enabled. The only outcome is that our custom nodes which have flat state **will not** run "peers" migration, but this is fine.

cc @saketh-are
nikurt pushed a commit to nikurt/nearcore that referenced this pull request Mar 15, 2023
this is a fix similar to
near#8711. After
near#8617, we need to correctly set the boot nodes in this test, otherwise we get a timeout error in test_tx_status() after the nodes are restarted
@saketh-are saketh-are mentioned this pull request Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants